r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

cc @RalfJung

somewhat suspicious - it's calling get_unchecked on a &str obtained via auto-deref which is zero-length now. So we more or less expect this needs to go through a raw pointer directly or something similar.

Yeah, that seems potentially problematic. Until we have a better idea of what the aliasing model will be long-term, it's better to be conservative and always remain in-bounds even for dynamically sized references.

I haven't carefully checked the code here, but a set-len-on-drop guard is a common approach elsewhere in the stdlib. Is this function covered by a liballoc test case? If yes, any idea why Miri is not complaining about this? If it is possible to cause a Miri error with the current implementation of that function, it'd be good to add that as a testcase.

The function was covered by this test, but it contains a catch_unwind (also added by #78499) that makes it not runnable in miri.

However even when run without the catch_unwind part it doesn't trigger miri. Heck, even the following program, which seems obviously wrong, doesn't trigger miri:

fn main() {
    unsafe {
        let mut v = vec![1, 2, 3];
        v.set_len(0);
        assert_eq!(*v.get_unchecked(1), 2);
        
        let a = [1, 2, 3];
        let s = &a[0..0];
        assert_eq!(s.len(), 0);
        assert_eq!(*s.get_unchecked(1), 2);
    }
}

I guess miri doesn't correctly track slices bounds but only the allocation ones?

but it contains a catch_unwind (also added by #78499) that makes it not runnable in miri.

Miri has supported unwinding for more than a year at this point. ;)

I guess miri doesn't correctly track slices bounds but only the allocation ones?

Interesting... I first thought this was just a missing -Zmiri-track-raw-pointers, but that's not it. The following does error though (with -Zmiri-track-raw-pointers):

fn main() {
    unsafe {
        let a = [1, 2, 3];
        let s = &a[0..1];
        assert_eq!(s.len(), 1);
        assert_eq!(*s.get_unchecked(1), 2);
    }
}

Something strange is going on with zero-sized references here.

Miri has supported unwinding for more than a year at this point. ;)

Oops, I copypasted the test without importing the panic module and got an error, then blindly assumed it was because Miri didn't support it. I feel dumb...

Heck, even the following program, which seems obviously wrong, doesn't trigger miri:

This is fixed now in latest Miri, but you need to pass -Zmiri-track-raw-pointers to get an error.

I'm a bit worried that the resulting code here is worse with a drop guard. Did you check?

It might be better to keep the set_len(0) and use std::slice::from_raw_parts and std::str::from_utf8_unchecked to create the slice.

Alternatively, you could keep use Vec::spare_capacity_mut right after set_len(0) together with slice_assume_init_mut() to get the full slice while temporarily having the Vec's len at zero. (You'd have to change the ptr::copy call though.)

I do already have most of an impl of the raw_parts and from_utf8_unchecked version in https://github.com/DJMcNab/retain_more/blob/c87ddd74b8b0c6660781dc9b5bb4b8f8a3187cc5/src/string.rs#L141-L223

I think the safety comments are correct, although the actual reasons that I think alloc::String::retain is broken are wrong. Obviously that impl can be simplified to not expose the additional power that it provides.

I'm a bit worried that the resulting code here is worse with a drop guard. Did you check?

The difference looks pretty minimal, it adds just a couple of methods and some instructions outside the main loop. https://godbolt.org/z/4rqM9P

I forgot to update the labels
@rustbot label -S-waiting-on-author +S-waiting-on-review

Also, I correct my previous statement, it doesn't add code to the function, it just moves the return label.

@bors r+

📌 Commit c89e643 has been approved by m-ou-se